Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] src,lib: policy permissions #33504

Closed
wants to merge 3 commits into from
Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented May 21, 2020

This is a new take at work @addaleax had previously gone through to define access control policies for Node.js. The original PR #22112 failed to make progress for a number of reasons.

This is a Work in progress that is not yet completed.

How does it work

There is no change to the default behavior of Node.js. Running the node binary with no command-line flags means that all permissions are granted by default as they are today.

Permissions may be denied or granted via command-line flags:

(example: deny network access)

$ node --policy-deny=net

A simple JavaScript API is provided to checking grants and applying a stricter policy:

if (!process.policy.check('net'))
  console.log('network apis are restricted');

// Deny file system modifications
process.policy.deny('fs.out');

There is no user-facing API for granting permissions from JavaScript.

All internal/* modules have access to a special runInPrivilegedScope() function that can be used to execute a synchronous function with no permission checking. For instance, suppose that Node.js is started without file system permissions (e.g. node --policy-deny=fs). Ordinarily this would mean that Node.js itself could not function because all file system permissions would be denied. The runInPriviledgedScope() utility (which is injected into internal modules the same way primordials and internalBinding is) makes it possible:

process.policy.deny('fs');
console.log(process.policy.check('fs.in')); // false!
console.log(process.policy.check('fs.out')); // false!

const { kPermissionFileSystemIn } = internalBinding('policy');
// available only inside internal/* module!
runInPrivilegedScope(() => {
  // run fs read operations here.
  console.log(process.policy.check('fs.in')); // true!
  console.log(process.policy.check('fs.out')); // true!
});

console.log(process.policy.check('fs.in')); // false!
console.log(process.policy.check('fs.out')); // false!

The runInPrivilegedScope() function can be bound such that...

function foo(a, b, c) {
  return a + b + c;
}

const privilegedFoo = runInPrivilegedScope.bind(this, foo);

console.log(privilegedFoo(1, 2, 3));

The function passed in to runInPrivilegedScope() is executed synchronously and the permission scope reverts as soon as the function returns. Care must be taken because any user code that is executed synchronously within the privileged scope will run with no permission checking.

At the C++ level, an equivalent mechanism is provided using the PrivilegedScope utility:

PrivilegedScope priv_scope(env);
// So long as priv_scope is in scope all permissions are granted

The Permission set is hierarchical. E.g. net, net.in, net.out. Denying or granting net denies or grants the entire branch.

If a denied API is invoked, a ERR_ACCESS_DENIED Node.js error would be thrown.

All permissions are set per process. Once denied, they are denied for the entire process, including all worker threads.

The permissions

  • inspector (controls access to the inspector API, implicitly denied)
  • addons (controls access to native addons, implicitly denied)
  • child_process (controls access to child processes, implicitly denied)
  • fs (controls access to file system)
    • fs.in (controls access to file system read-operations)
    • fs.out (controls access to file system write-operations)
  • net (controls access to network access)
    • net.in (controls access to network servers)
    • net.out (controls access to network clients)
  • wasi (controls access to experimental WASI)
  • process (controls access to APIs that manipulate process state)
  • signal (controls access to sending signals to other processes)
  • timing (controls access to high resolution timing APIs)
  • env (controls access to environment / user info)
  • workers (controls access to worker threads)
  • policy (controls access to policy permission checking)

Implicitly denied permissions are denied automatically when any other permission is denied.

Denying permissions within a scope

Given the way that runInPrivilegedScope() works, A denied permission will only be in effect while the scope is active. So, for instance, if I launch Node.js with net enabled, then call runInPrivilegedScope() and subsequently deny net, once the privileged scope pops off the stack net will be permitted again. This should be obvious but it's worth calling out.

The question is whether it should be possible to deny a permission in parent scopes from a child. To keep things simple, I would say no.

Additional work to come

The previous version of this PR included all of the module changes to enforce the policy. I've backed those out so we can focus first on the core mechanism here. I will be reapplying those changes later.

FAQ

  1. Is this a security mechanism?

This is a key question. I don't want to classify this as a security thing. It's a capability-centered policy configuration. Security for your Node.js process really needs to come from the environment (e.g. running the Node.js process in a sandboxed container or running under a restricted uid, etc). What this mechanism does is selectively disable certain APIs in the standard library so that they are not available to running scripts. One use case, for instance, would be running utilities like npx to run scripts that should not necessarily have the ability to open network connections by default.

  1. Is this just like deno's permissions?

Yes and no. Yes in that they cover the same fundamental ideas. No in that it uses a different way of expressing permissions and is not advertised as a security mechanism. For deno, for instance, you enable file system reads for specific paths. Here, file system access is either on or off.

And in case someone is wondering if this is opened in response to deno having this, work on this mechanism started a long time ago in #22112 but was put on hold and we've been meaning to revisit it. We (NearForm) have had some use cases for this for a while that we're just now coming back around to so we wanted to move it forward.

I don't currently have plans to align it with deno's permission model but that's obviously something we could discuss if there would be enough ecosystem benefit to do so and it covers all the use cases.

  1. Why not support grant requests at runtime?

The ability to grant permissions at runtime is ruled out here to prevent the ability for a script to maliciously elevate it's permissions. Because this information is stored in the V8 context, it would be possible for a native addon to manipulate this permissions so if any permissions are denied we automatically also deny native addons by default but when native addons are allowed it would be easy to manipulate things.

In most of the use scenarios we've modeled for using this, the ability to request a permission just never was a consideration. Imagine, for instance, code running on a server. These modules are going to be known to the developer, what capabilities they need will be known, and there just won't be a need to enable any kind of dynamic request mechanism.

  1. Have you benchmarked this yet?

Hahahah... um... no, not yet. Will get to that a bit later.

What Now?

I am opening this PR to start the discussion and the iteration on these ideas. Feedback is welcome and requested.

/cc @addaleax @mcollina

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 21, 2020
@jasnell jasnell requested a review from addaleax May 21, 2020 22:55
@guybedford
Copy link
Contributor

Great to see this initiative. Has there been any discussion over per-package permissions? What is the best forum to engage in these discussions?

Since Node.js has a well-defined concept of package boundaries with the package.json lookup, it is possible to consider fine-grained permissions models per-package, but it would involve providing a tailored builtin-module instance through the resolver reflecting the given policy.

I know this is very different to the base implemented here, but if it were to be considered a possible future extension, perhaps there are ways to design the policy format in a way that would lend itself to package-based permissioning in this form?

src/env-inl.h Outdated Show resolved Hide resolved
src/policy/policy-inl.h Outdated Show resolved Hide resolved
src/policy/policy-inl.h Outdated Show resolved Hide resolved
src/policy/policy-inl.h Outdated Show resolved Hide resolved
src/policy/policy.h Outdated Show resolved Hide resolved
@jasnell
Copy link
Member Author

jasnell commented May 21, 2020

@guybedford

Has there been any discussion over per-package permissions?

It would be possible under the model proposed here if, for instance, modules were loaded in their own V8 contexts, which is certainly possible but introduces it's own set of complexities. Imagine an API such as:

const vm = require('vm')
const mod = vm.require('foo', { policy: { deny: 'fs' } })

Or, when using ESM semantics:

vm.import('foo', { policy: { deny: 'fs' } }).then((mod) => { /*..*/ });

These each have their own issues and wouldn't be perfect sandboxes by any stretch as the V8 context can be escaped but it would be a path forward.

@guybedford
Copy link
Contributor

guybedford commented May 22, 2020

What I was considering is something like imagining having import 'fs' itself actual fail in the resolver as opposed to when trying to call the APIs. This does get into the slippery slope of "capability reachability" in that you can be passed an fs permission from another package. But capability delegation is largely considered a good pattern in capability thinking as far as I'm aware.

So whether a package can do import { readFile } from 'fs' could be thrown by the resolver since it knows who is doing the resolution. Dynamic import is similarly scoped.

Don't mean to get too off-topic, but it's an interesting discussion. I suppose the questions are if it is worth even considering and if so what might help gain alignment here with it. I'm open to all options on both questions :)

@bengl
Copy link
Member

bengl commented May 22, 2020

I think this is a move in the right direction! 👍

A few things:

  1. Could this end up having more finer-grained policies? For example, perhaps the TCP policy could restrict ports or IPs to connect to? Also if specific addons/child-processes (and arguments) are allowed, this might make it easier to allow "special" APIs and other APIs to both be allowed. (If somehow, this could also be done for higher level APIS, maybe by passing a token around, this would be pretty great.)
  2. I wonder if net policies can be sidestepped through fs or vice-versa? Either way, finer-grained policies would mitigate that.
  3. I need to think a little harder about this, but I'm pretty sure that policies need to be (at least) consistent across the whole Isolate and any Contexts within it, user-created or otherwise. Same goes for workers, I think. Things that can freely communicate can't really have different policies.

Also, the prior discussion from the Security WG is worth looking at, for anyone who hasn't seen it (or the many things it links to).

/cc @deian

@jasnell
Copy link
Member Author

jasnell commented May 22, 2020

Could this end up having more finer-grained policies? For example, perhaps the TCP policy could restrict ports or IPs to connect to? Also if specific addons/child-processes (and arguments) are allowed, this might make it easier to allow "special" APIs and other APIs to both be allowed. (If somehow, this could also be done for higher level APIS, maybe by passing a token around, this would be pretty great.)

Potentially, although I would hesitate to try expressing those via command-line arguments. If we wanted to enable something more fine grained then we should expand on the policy.json document used by the integrity policy feature. That could be used to express things like ports, addresses, file system paths, etc with greater detail.

I wonder if net policies can be sidestepped through fs or vice-versa? Either way, finer-grained policies would mitigate that.

Yes, possibly. When the net permission is denied we may need to stat fd's to make sure.

I need to think a little harder about this, but I'm pretty sure that policies need to be (at least) consistent across the whole Isolate and any Contexts within it, user-created or otherwise. Same goes for workers, I think. Things that can freely communicate can't really have different policies.

workers / contexts created by the current context will (don't currently) have the current policy as a baseline but may have more restrictive policies.

@bengl
Copy link
Member

bengl commented May 22, 2020

Could this end up having more finer-grained policies?

Potentially, although I would hesitate to try expressing those via command-line arguments. If we wanted to enable something more fine grained then we should expand on the policy.json document used by the integrity policy feature. That could be used to express things like ports, addresses, file system paths, etc with greater detail.

💯 I'm all for this approach.

I need to think a little harder about this, but I'm pretty sure that policies need to be (at least) consistent across the whole Isolate and any Contexts within it, user-created or otherwise. Same goes for workers, I think. Things that can freely communicate can't really have different policies.

workers / contexts created by the current context will (don't currently) have the current policy as a baseline but may have more restrictive policies.

Since contexts currently don't have access to require or process on their own (AFAIK that hasn't changed?), any access to Node APIs inside a context would have to be provided from the main context anyway, so wouldn't that make it effectively the same policy? Apart from that, if they do have restricted policies, wouldn't they still be able to expand privilege if they get access to objects from the main Node context?

@targos
Copy link
Member

targos commented May 22, 2020

This fails to compile with GCC:

c++ -MMD -MF obj/src/policy/libnode.policy.o.d -DV8_DEPRECATION_WARNINGS -DV8_IMMINENT_DEPRECATION_WARNINGS -D__STDC_FORMAT_MACROS -DOPENSSL_NO_PINSHARED -DOPENSSL_THREADS '-DNODE_ARCH="x64"' '-DNODE_PLATFORM="linux"' -DNODE_WANT_INTERNALS=1 -DV8_DEPRECATION_WARNINGS=1 '-DNODE_OPENSSL_SYSTEM_CERT_PATH=""' -DHAVE_INSPECTOR=1 -DNODE_ENABLE_LARGE_CODE_PAGES=1 -D__POSIX__ -DNODE_USE_V8_PLATFORM=1 -DNODE_HAVE_I18N_SUPPORT=1 -DHAVE_OPENSSL=1 -DUCONFIG_NO_SERVICE=1 -DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION=1 -DU_HAVE_STD_STRING=1 -DUCONFIG_NO_BREAK_ITERATION=0 -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -D_POSIX_C_SOURCE=200112 -DNGHTTP2_STATICLIB -I../../src -Igen -Igen/include -Igen/src -I../../deps/histogram/src -I../../deps/uvwasi/include -I../../deps/v8/include -I../../deps/icu-small/source/i18n -I../../deps/icu-small/source/common -I../../deps/zlib -I../../deps/llhttp/include -I../../deps/cares/include -I../../deps/uv/include -I../../deps/nghttp2/lib/includes -I../../deps/brotli/c/include -I../../deps/openssl/openssl/include -Wall -Wextra -Wno-unused-parameter -pthread -Wall -Wextra -Wno-unused-parameter -m64 -O3 -fno-omit-frame-pointer -fno-rtti -fno-exceptions -std=gnu++1y  -c ../../src/policy/policy.cc -o obj/src/policy/libnode.policy.o
../../src/policy/policy.cc:66:23: error: pasting "->" and "Special" does not give a valid preprocessing token
   66 |   binding_data->detail->##name = policy->granted(Permissions::k##name) ? 1 : 0;
      |                       ^~
../../src/policy/policy.h:19:3: note: in expansion of macro ‘V’
   19 |   V(Special, "special", PermissionsRoot)                                       \
      |   ^
../../src/policy/policy.cc:67:3: note: in expansion of macro ‘PERMISSIONS’
   67 |   PERMISSIONS(V)
      |   ^~~~~~~~~~~
../../src/policy/policy.cc:66:23: error: pasting "->" and "SpecialInspector" does not give a valid preprocessing token
   66 |   binding_data->detail->##name = policy->granted(Permissions::k##name) ? 1 : 0;
      |                       ^~
../../src/policy/policy.h:20:3: note: in expansion of macro ‘V’
   20 |   V(SpecialInspector, "special.inspector", Special)                            \
      |   ^
../../src/policy/policy.cc:67:3: note: in expansion of macro ‘PERMISSIONS’
   67 |   PERMISSIONS(V)
      |   ^~~~~~~~~~~
../../src/policy/policy.cc:66:23: error: pasting "->" and "SpecialAddons" does not give a valid preprocessing token
   66 |   binding_data->detail->##name = policy->granted(Permissions::k##name) ? 1 : 0;
      |                       ^~
../../src/policy/policy.h:21:3: note: in expansion of macro ‘V’
   21 |   V(SpecialAddons, "special.addons", Special)                                  \
      |   ^
../../src/policy/policy.cc:67:3: note: in expansion of macro ‘PERMISSIONS’
   67 |   PERMISSIONS(V)
      |   ^~~~~~~~~~~
../../src/policy/policy.cc:66:23: error: pasting "->" and "SpecialChildProcess" does not give a valid preprocessing token
   66 |   binding_data->detail->##name = policy->granted(Permissions::k##name) ? 1 : 0;
      |                       ^~
../../src/policy/policy.h:22:3: note: in expansion of macro ‘V’
   22 |   V(SpecialChildProcess, "special.child_process", Special)                     \
      |   ^
../../src/policy/policy.cc:67:3: note: in expansion of macro ‘PERMISSIONS’
   67 |   PERMISSIONS(V)
      |   ^~~~~~~~~~~
../../src/policy/policy.cc:66:23: error: pasting "->" and "Workers" does not give a valid preprocessing token
   66 |   binding_data->detail->##name = policy->granted(Permissions::k##name) ? 1 : 0;
      |                       ^~
../../src/policy/policy.h:23:3: note: in expansion of macro ‘V’
   23 |   V(Workers, "workers", PermissionsRoot)                                       \
      |   ^
../../src/policy/policy.cc:67:3: note: in expansion of macro ‘PERMISSIONS’
   67 |   PERMISSIONS(V)
      |   ^~~~~~~~~~~
../../src/policy/policy.cc:66:23: error: pasting "->" and "FileSystem" does not give a valid preprocessing token
   66 |   binding_data->detail->##name = policy->granted(Permissions::k##name) ? 1 : 0;
      |                       ^~
../../src/policy/policy.h:24:3: note: in expansion of macro ‘V’
   24 |   V(FileSystem, "fs", PermissionsRoot)                                         \
      |   ^
../../src/policy/policy.cc:67:3: note: in expansion of macro ‘PERMISSIONS’
   67 |   PERMISSIONS(V)
      |   ^~~~~~~~~~~
../../src/policy/policy.cc:66:23: error: pasting "->" and "FileSystemIn" does not give a valid preprocessing token
   66 |   binding_data->detail->##name = policy->granted(Permissions::k##name) ? 1 : 0;
      |                       ^~
../../src/policy/policy.h:25:3: note: in expansion of macro ‘V’
   25 |   V(FileSystemIn, "fs.in", FileSystem)                                         \
      |   ^
../../src/policy/policy.cc:67:3: note: in expansion of macro ‘PERMISSIONS’
   67 |   PERMISSIONS(V)
      |   ^~~~~~~~~~~
../../src/policy/policy.cc:66:23: error: pasting "->" and "FileSystemOut" does not give a valid preprocessing token
   66 |   binding_data->detail->##name = policy->granted(Permissions::k##name) ? 1 : 0;
      |                       ^~
../../src/policy/policy.h:26:3: note: in expansion of macro ‘V’
   26 |   V(FileSystemOut, "fs.out", FileSystem)                                       \
      |   ^
../../src/policy/policy.cc:67:3: note: in expansion of macro ‘PERMISSIONS’
   67 |   PERMISSIONS(V)
      |   ^~~~~~~~~~~
../../src/policy/policy.cc:66:23: error: pasting "->" and "User" does not give a valid preprocessing token
   66 |   binding_data->detail->##name = policy->granted(Permissions::k##name) ? 1 : 0;
      |                       ^~
../../src/policy/policy.h:27:3: note: in expansion of macro ‘V’
   27 |   V(User, "user", PermissionsRoot)                                             \
      |   ^
../../src/policy/policy.cc:67:3: note: in expansion of macro ‘PERMISSIONS’
   67 |   PERMISSIONS(V)
      |   ^~~~~~~~~~~
../../src/policy/policy.cc:66:23: error: pasting "->" and "Net" does not give a valid preprocessing token
   66 |   binding_data->detail->##name = policy->granted(Permissions::k##name) ? 1 : 0;
      |                       ^~
../../src/policy/policy.h:28:3: note: in expansion of macro ‘V’
   28 |   V(Net, "net", PermissionsRoot)                                               \
      |   ^
../../src/policy/policy.cc:67:3: note: in expansion of macro ‘PERMISSIONS’
   67 |   PERMISSIONS(V)
      |   ^~~~~~~~~~~
../../src/policy/policy.cc:66:23: error: pasting "->" and "NetUdp" does not give a valid preprocessing token
   66 |   binding_data->detail->##name = policy->granted(Permissions::k##name) ? 1 : 0;
      |                       ^~
../../src/policy/policy.h:29:3: note: in expansion of macro ‘V’
   29 |   V(NetUdp, "net.udp", Net)                                                    \
      |   ^
../../src/policy/policy.cc:67:3: note: in expansion of macro ‘PERMISSIONS’
   67 |   PERMISSIONS(V)
      |   ^~~~~~~~~~~
../../src/policy/policy.cc:66:23: error: pasting "->" and "NetDNS" does not give a valid preprocessing token
   66 |   binding_data->detail->##name = policy->granted(Permissions::k##name) ? 1 : 0;
      |                       ^~
../../src/policy/policy.h:30:3: note: in expansion of macro ‘V’
   30 |   V(NetDNS, "net.dns", Net)                                                    \
      |   ^
../../src/policy/policy.cc:67:3: note: in expansion of macro ‘PERMISSIONS’
   67 |   PERMISSIONS(V)
      |   ^~~~~~~~~~~
../../src/policy/policy.cc:66:23: error: pasting "->" and "NetTCP" does not give a valid preprocessing token
   66 |   binding_data->detail->##name = policy->granted(Permissions::k##name) ? 1 : 0;
      |                       ^~
../../src/policy/policy.h:31:3: note: in expansion of macro ‘V’
   31 |   V(NetTCP, "net.tcp", Net)                                                    \
      |   ^
../../src/policy/policy.cc:67:3: note: in expansion of macro ‘PERMISSIONS’
   67 |   PERMISSIONS(V)
      |   ^~~~~~~~~~~
../../src/policy/policy.cc:66:23: error: pasting "->" and "NetTCPIn" does not give a valid preprocessing token
   66 |   binding_data->detail->##name = policy->granted(Permissions::k##name) ? 1 : 0;
      |                       ^~
../../src/policy/policy.h:32:3: note: in expansion of macro ‘V’
   32 |   V(NetTCPIn, "net.tcp.in", NetTCP)                                            \
      |   ^
../../src/policy/policy.cc:67:3: note: in expansion of macro ‘PERMISSIONS’
   67 |   PERMISSIONS(V)
      |   ^~~~~~~~~~~
../../src/policy/policy.cc:66:23: error: pasting "->" and "NetTCPOut" does not give a valid preprocessing token
   66 |   binding_data->detail->##name = policy->granted(Permissions::k##name) ? 1 : 0;
      |                       ^~
../../src/policy/policy.h:33:3: note: in expansion of macro ‘V’
   33 |   V(NetTCPOut, "net.tcp.out", NetTCP)                                          \
      |   ^
../../src/policy/policy.cc:67:3: note: in expansion of macro ‘PERMISSIONS’
   67 |   PERMISSIONS(V)
      |   ^~~~~~~~~~~
../../src/policy/policy.cc:66:23: error: pasting "->" and "NetTLS" does not give a valid preprocessing token
   66 |   binding_data->detail->##name = policy->granted(Permissions::k##name) ? 1 : 0;
      |                       ^~
../../src/policy/policy.h:34:3: note: in expansion of macro ‘V’
   34 |   V(NetTLS, "net.tls", Net)                                                    \
      |   ^
../../src/policy/policy.cc:67:3: note: in expansion of macro ‘PERMISSIONS’
   67 |   PERMISSIONS(V)
      |   ^~~~~~~~~~~
../../src/policy/policy.cc:66:23: error: pasting "->" and "NetTLSLog" does not give a valid preprocessing token
   66 |   binding_data->detail->##name = policy->granted(Permissions::k##name) ? 1 : 0;
      |                       ^~
../../src/policy/policy.h:35:3: note: in expansion of macro ‘V’
   35 |   V(NetTLSLog, "net.tls.log", NetTLS)                                          \
      |   ^
../../src/policy/policy.cc:67:3: note: in expansion of macro ‘PERMISSIONS’
   67 |   PERMISSIONS(V)
      |   ^~~~~~~~~~~
../../src/policy/policy.cc:66:23: error: pasting "->" and "Process" does not give a valid preprocessing token
   66 |   binding_data->detail->##name = policy->granted(Permissions::k##name) ? 1 : 0;
      |                       ^~
../../src/policy/policy.h:36:3: note: in expansion of macro ‘V’
   36 |   V(Process, "process", PermissionsRoot)                                       \
      |   ^
../../src/policy/policy.cc:67:3: note: in expansion of macro ‘PERMISSIONS’
   67 |   PERMISSIONS(V)
      |   ^~~~~~~~~~~
../../src/policy/policy.cc:66:23: error: pasting "->" and "Timing" does not give a valid preprocessing token
   66 |   binding_data->detail->##name = policy->granted(Permissions::k##name) ? 1 : 0;
      |                       ^~
../../src/policy/policy.h:37:3: note: in expansion of macro ‘V’
   37 |   V(Timing, "timing", PermissionsRoot)                                         \
      |   ^
../../src/policy/policy.cc:67:3: note: in expansion of macro ‘PERMISSIONS’
   67 |   PERMISSIONS(V)
      |   ^~~~~~~~~~~
../../src/policy/policy.cc:66:23: error: pasting "->" and "Signal" does not give a valid preprocessing token
   66 |   binding_data->detail->##name = policy->granted(Permissions::k##name) ? 1 : 0;
      |                       ^~
../../src/policy/policy.h:38:3: note: in expansion of macro ‘V’
   38 |   V(Signal, "signal", PermissionsRoot)                                         \
      |   ^
../../src/policy/policy.cc:67:3: note: in expansion of macro ‘PERMISSIONS’
   67 |   PERMISSIONS(V)
      |   ^~~~~~~~~~~
../../src/policy/policy.cc:66:23: error: pasting "->" and "Experimental" does not give a valid preprocessing token
   66 |   binding_data->detail->##name = policy->granted(Permissions::k##name) ? 1 : 0;
      |                       ^~
../../src/policy/policy.h:39:3: note: in expansion of macro ‘V’
   39 |   V(Experimental, "experimental", PermissionsRoot)                             \
      |   ^
../../src/policy/policy.cc:67:3: note: in expansion of macro ‘PERMISSIONS’
   67 |   PERMISSIONS(V)
      |   ^~~~~~~~~~~
../../src/policy/policy.cc:66:23: error: pasting "->" and "ExperimentalWasi" does not give a valid preprocessing token
   66 |   binding_data->detail->##name = policy->granted(Permissions::k##name) ? 1 : 0;
      |                       ^~
../../src/policy/policy.h:40:3: note: in expansion of macro ‘V’
   40 |   V(ExperimentalWasi, "experimental.wasi", Experimental)
      |   ^
../../src/policy/policy.cc:67:3: note: in expansion of macro ‘PERMISSIONS’
   67 |   PERMISSIONS(V)
      |   ^~~~~~~~~~~

Copy link
Member

@vdeturckheim vdeturckheim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am +1 on this

doc/api/policy.md Outdated Show resolved Hide resolved
@devsnek
Copy link
Member

devsnek commented May 22, 2020

I'm also more of a fan of the approach Guy mentioned, it would be interesting to explore more OCAP within node.

@GeoffreyBooth
Copy link
Member

I'm also more of a fan of the approach Guy mentioned

This also strikes me as a great benefit of Node over Deno, in that we do have packages and package scopes. A common scenario that I can imagine myself taking advantage of is to somehow configure Node such that my app has full permissions, but packages have no write or network access. In other words, I trust my code, and Node builtins, but not third-party code. In Node we do have the concept of the boundary between application code and third-party code, so we should take advantage of it.

@jasnell
Copy link
Member Author

jasnell commented May 23, 2020

Introducing an ocap model at the package layer is going to be a significantly more involved and fundamentally invasive activity. The arrangement of our core apis across core modules is inconsistent and does not lend itself easily to that model. For instance, both process.dlopen() and process.hrtime() exist on the same module and are two very different capabilities. While I do not doubt that loading modules in protected scopes is an interesting problem, this pr is not looking to tackle that problem.

@mcollina
Copy link
Member

Potentially, although I would hesitate to try expressing those via command-line arguments. If we wanted to enable something more fine grained then we should expand on the policy.json document used by the integrity policy feature. That could be used to express things like ports, addresses, file system paths, etc with greater detail.

I think this would be fantastic to do. A significant use case would be the "build" step in npm install.

@jasnell
Copy link
Member Author

jasnell commented May 23, 2020

@guybedford @devsnek @GeoffreyBooth ... let's move discussions around module-scoped permissions/capabilities out of this PR and into it's own thread as it really is a much different and much more difficult topic to explore. In this PR, I am specifically looking at coarse-grained use restrictions around process, context, and worker thread level core-api capabilities (i.e. "is this API available for use in this process or not?") and will subsequently look at a separate PR that leverages the policy.json mechanism to express more fine-grained use restrictions (network ports, addresses, file system paths, etc).

@vdeturckheim:

A script can always try...catch around a call that might fail due to a policy denial. But in the case of a untrusted script, I'd prefer to have a way to prevent it from running if it tries to do anything I don't want.

So thinking about this, it could actually be a special permission on it's own. Specifically, something like:

$ node --policy-deny=policy

Denying the policy permission would do two things: 1) it would restrict access to the process.policy.granted() API and 2) it would cause the process to exit immediately instead of throwing, not giving the user code the opportunity to handle the error. Note that it would not forbid the use of the process.policy.deny() API as I think we always want the ability to have the policy further restricted.

@vdeturckheim
Copy link
Member

@jasnell

Denying the policy permission would do two things: 1) it would restrict access to the process.policy.granted() API and 2) it would cause the process to exit immediately instead of throwing, not giving the user code the opportunity to handle the error. Note that it would not forbid the use of the process.policy.deny() API as I think we always want the ability to have the policy further restricted.

This would be perfect for the defense model I had in mind and makes a lot of sense!

@vdeturckheim
Copy link
Member

Also, I'd say we should move forward on this and leave OCAP for another iteration. I have experimented a bit with it and I found it was very hard to make it right. Having process level policy right now is, IMO, a good move as it provides value to users they can use quickly and give us a great base to iterate later.

@bengl
Copy link
Member

bengl commented May 25, 2020

Interestingly, if you deny fs (or even just fs.in) from the command line, you cannot run any scripts:

$ ./node --policy-deny=fs script.js
fs.js:1650
      const stats = binding.lstat(baseLong, false, undefined, ctx);
                            ^

Error: Access to this API has been restricted
    at Object.realpathSync (fs.js:1650:29)
    at toRealPath (internal/modules/cjs/loader.js:369:13)
    at Function.Module._findPath (internal/modules/cjs/loader.js:673:22)
    at resolveMainPath (internal/modules/run_main.js:12:25)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:65:24)
    at internal/main/run_main_module.js:17:47 {
  code: 'ERR_ACCESS_DENIED'
}

While this makes sense because module loading uses fs, this might seem a little unexpected.

This is support for taking one of a few different approaches:

  1. Allow internal JS code to bypass policies in some cases, by providing unencumbered copies of the APIs, or passing some internal security token, either way only available via internalBinding(). This is of course prone to mistakes that might leak the unencumbered APIs to userland, which would render any associated policies inert.
  2. Have a separate set of policies (maybe the existing integrity-check policies) govern require, and have only module loading bypass fs policies.
  3. Providing finer-grained policies would also enable the same thing.
  4. Don't worry about it, since it's sound as it stands, even if it's a bit strange.

Relatedly, doing a process.policy.deny('fs.out') blocks any writing to stdout. This breaks (among other things) the REPL. Again, this makes sense, but might be a little unexpected. Maybe exceptions could be made for the std[in|out|err] file descriptors? Or have them treated separately?

@jasnell
Copy link
Member Author

jasnell commented May 25, 2020

@bengl yeah I spotted that also. We have a number of such issues currently (e.g. all of our internal uses of process.env). I've been thinking through a number of approaches on this. Internal privileged operations being one possibility. Another would be to implement a kind of run level type of mechanism, where node bootstrap or internal code runs at one run level, and user code runs at another. The permissions would then be applied at the user code run level. That however gets quite a bit more complicated, so I am still evaluating the pros and the cons.

@mcollina
Copy link
Member

Note that require is a double-edge sword. Essentially you want to require the modules your application depends on, but nothing more. I think require might really be impossible to secure :/. An option might be to apply the policies after the event loop has done a first turn, allowing the application to start and load its modules (no dynamic require/import)

Does this happen with esm? I think we can easily white-list the syntax version as it happens before loading everything. (loaders must be fully privileged anyway).

@bengl
Copy link
Member

bengl commented May 26, 2020

@mcollina

Essentially you want to require the modules your application depends on, but nothing more. I think require might really be impossible to secure :/. An option might be to apply the policies after the event loop has done a first turn, allowing the application to start and load its modules (no dynamic require/import)

That approach would probably have to be restricted to entirely ESM module trees.

Another approach here is to use the existing integrity checks as a baseline allow-list for finer-grained fs policies.

Does this happen with esm? I think we can easily white-list the syntax version as it happens before loading everything. (loaders must be fully privileged anyway).

As it currently stands with this PR, you can't load an mjs file as your main script if you have --policy-deny=fs, and even if you do a process.policy.deny('fs') after loading, other code doing nefarious things may already have run (if I understand ESM module loading correctly, which I might not). If we're able to drop file reading privileges after module loading but before any app/userland code has run, then maybe this might work, but only for ESM.

@mcollina
Copy link
Member

I totally agree, this would be a clear advantage of esm.

@ljharb
Copy link
Member

ljharb commented May 27, 2020

cc @nodejs/modules-active-members, since this seems to impact it?

@bengl
Copy link
Member

bengl commented May 27, 2020

@mcollina

I totally agree, this would be a clear advantage of esm.

To be clear: it would be more ideal, I think, to ensure that existing CommonJS code could still be restricted from fs usage. This is why I think finer-grained fs policies are necessary.

If there's a UX concern, they can be implicitly finer-grained by simplying allowing files to be read if they're in the Integrity Checks JSON file. That being said, allowing user control over what can be read/written would allow for other use cases too.

@jasnell
Copy link
Member Author

jasnell commented May 27, 2020

My next step on this will be to do a more complete write-up of the underlying model, the expectations, requirements that I see, etc. One important point on this: the initial version of this is INTENDED to be imperfect and experimental and the goal is to not solve every potential problem out of the gate before it lands but to give us something that we can iterate on and improve over time. In other words, it's an experimental feature that we will absolutely not get immediately correct.

@mcollina
Copy link
Member

Agreed!

@guybedford
Copy link
Contributor

To try and put together a more concrete suggestion from my previous comment, one way in which we could lay the path for modular capabilities might be to consider having an architecture for builtin factories:

const fs = createFSBuiltin(permissions);

// create an fs builtin gated to a specific folder on the filesystem:
const fsLocal = createFSBuiltin({ gatedFolder: '/path/to/project' });

// create an fs builtin with only read capability:
const fsRead = createFSBuiltin({ read: true });

When a user does require('fs') or import('fs') they would then get the permissioned-instance of the builtin, different from the base high-level one. So treating permissions as a sort of bound wrapper around the builtin, instead of checks within the original core code itself.

I'm not saying this should be exposed to userland, but rather that as an overall architecture approach internally it might be a path to the compartment / modular security models.

This also gets around the problem of the internal fs functions having the same permissions as the rest of the world, since the permission system can be reflected in the instance boundary itself, while internal core code continues to use the high permission internal instance.

To add modular restriction to such a model would then just be having a resolver that returns the appropriately permissioned builtin. I'd be interested to hear if this works with the constraints @bengl imagines here too.

There are likely edge case complexities like the fact that realpath can move outside of gated folders, so the gating checks would need to be associated with the call invocation itself rather than being environment-level permissions check, but that probably isn't too bad? I certainly understand if that's an argument for such a model again being a future improvement.

@bengl
Copy link
Member

bengl commented Jun 23, 2020

@guybedford

To add modular restriction to such a model would then just be having a resolver that returns the appropriately permissioned builtin. I'd be interested to hear if this works with the constraints @bengl imagines here too.

There are likely edge case complexities like the fact that realpath can move outside of gated folders, so the gating checks would need to be associated with the call invocation itself rather than being environment-level permissions check, but that probably isn't too bad? I certainly understand if that's an argument for such a model again being a future improvement.

(Sorry for the late reply!)

Gating on the call invocation itself would certainly be a start, but it's still missing the fact that resource access can be granted to module A, denied to module B, but then have that resource passed from A to B after the call has already been made.

That being said, per-policy-set builtins, as you've described them, seems reasonable as long there's actual isolation between policy sets.


This PR opts for binding-layer checks, and I think it should be allowed to proceed in that fashion, because regardless of how this ends up looking, permissions enforcement ought to be transparent to user code.

@jasnell
Copy link
Member Author

jasnell commented Oct 2, 2020

@addalex @bengl @guybedford @targos @devsnek @vdeturckheim @mcollina ... I've started picking this back up again. I've taken a step back and reworked the central implementation and took out the actual policy enforcement for the time being so we can focus on the central mechanism. I've updated the description in the pull request description. Please take a look when you have a moment

src/node.cc Outdated Show resolved Hide resolved
lib/internal/process/policy.js Show resolved Hide resolved
lib/internal/process/policy.js Outdated Show resolved Hide resolved
@andreineculau
Copy link

Definitely a move in the right direction! 👏

  1. Have you considered a hook-based implementation and skip the whole static declaration ?

For instance we're talking about different types of granularity in #33504 (comment) and #33504 (comment) . If you have hooks for importing * or an export (function or not!) from a module, you cover that granularity. If you have hooks for gating the call of a module's function, you cover that granularity as well.

A knee-jerk API just for illustration purposes could look like:

// policy.js // everything is allowed in here: fs, net, etc

const somePkgImportPolicy = require('some-pkg');

export const import = function({
  module /* {id, filename, exports, ...} */,
  pkg, /* optional, package.json contents or {name: '..'} for internal modules */
  exportName
}) {
  if (!somePkgImportPolicy(...arguments)) return false;
  if (pkg.name === 'fs') return false;
  if (/node_modules\/lodash/.test(module.filename)) return false;
  ...
  return true;
}

export const call = function({
  module,
  pkg,
  functionName,
  functionArgs
}) {
  if (pkg.name === 'fs' && functionName === 'readSync') return true;
  if (pkg.name === 'fs' && functionName === 'writeSync' && /^/tmp\//.test(functionArgs[0])) return true;
  if (pkg.name === 'internal' && functionName === 'fs.in') return false;
  if (pkg.name === 'internal' && functionName === 'fs.out' return false;
  ...
  return true;
}

I've made a similar argument in the pnpm world, and I hope @zkochan still holds to his words that it was a great idea :) 👋 https://medium.com/pnpm/why-package-managers-need-hook-systems-b8125d8b3dc7

  1. I sneaked the somePkgImportPolicy in my example above because I guess that would cover the per-package discussion as well in the sense that you would want to keep duplication at bay: if a package says it needs to write to some folder, then so be it, I will import its policy and obey it because I trust the package. I can't imagine you'd want to obey all of the packages' policies just because you depend on them, and not have any power - you as the node process' initiator.

PS: I can already imagine cons with the hook system in this specific context of policies, but I'd suggest focusing a bit on the pros of it and only later on possible cons and mitigations.

@jasnell
Copy link
Member Author

jasnell commented Dec 11, 2020

All, I've updated the implementation here with a few significant changes...

  1. The policy is now set per process and is largely independent of the Environment. Any call to process.policy.deny() at runtime will impact the entire process, including worker threads. Similar to other APIs that impact process-level state, we might want to restrict deny() to the main thread.

  2. The runInPrivilegedScope() and PrivilegedScope utilities now just disable the permission checking entirely. Previously these would be called with a list of denied and granted permissions that would apply in that scope. The new approach is a simplification but we'd need to figure out if it's the right one. Note that we'll have to be careful not to call user code while in a privileged scope or else that code will run without permission checking.

  3. The permissions have been simplified.

There are still a ton of unanswered questions in this, uncluding whether we want this kind of thing at all.

@jasnell
Copy link
Member Author

jasnell commented Dec 14, 2020

@andreineculau ... the hook-based approach looks interesting but it also looks much more complicated -- in a way that I'm not sure is justified. To be used correctly the permissions mechanism really ought to be as simple as possible -- even if that borders on the simplistic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.